Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: dai-zephyr: copy data to all available sinks #9200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnikodem
Copy link
Contributor

@dnikodem dnikodem commented Jun 5, 2024

In the case where the copier has more than one output and, additionally, we will be using a pin other than pin 0 (for example, when we want to copy data from pin 1 of a gateway-type copier to the next pipeline), it is necessary to copy data in playback path from the source to the output for all possible sinks.

@dnikodem dnikodem force-pushed the copy_audio_to_all_available_sinks branch 3 times, most recently from 2649bd8 to 3c13bc4 Compare June 6, 2024 11:42
In the case where the copier has more than one output and,
additionally, we will be using a pin other than pin 0
(for example, when we want to copy data from pin 1 of
a gateway-type copier to the next pipeline), it is necessary
to copy data in playback path from the source to the output
for all possible sinks.

Signed-off-by: Damian Nikodem <[email protected]>
@dnikodem dnikodem force-pushed the copy_audio_to_all_available_sinks branch from 3c13bc4 to 9f5f8ce Compare June 12, 2024 11:35
@dnikodem dnikodem marked this pull request as ready for review June 17, 2024 09:59
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@serhiy-katsyuba-intel serhiy-katsyuba-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dai_common_copy() calculates the amount of bytes that should be copied. For playback case that is line 1525. Seems that code should be modified to also check if there is free space in all sink buffers, current version only checks for space available in source and free in dma_buffer.


sink = container_of(sink_list, struct comp_buffer, source_list);

if (sink == dd->dma_buffer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dma_buffer is not on the list of sinks, so this condition is always false. However, maybe it is OK to leave it as is or replace with assert() "just in case" someone add dma_buffer to sink list in some future refactoring.

continue;
}

if (sink_dev && sink_dev->state == COMP_STATE_ACTIVE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those additional sinks are (always?) connected to other pipelines. And so some other pipelines might be responsible to set buffer stream params in .params() handler of components connected to those sinks. For unknown reason checking sink_dev->state == COMP_STATE_ACTIVE is not enough, I recently observed problems with multiple-pause-resume test on my draft PR and have to fix it like this: 4ebd807.

Frame format of uninitialized buffer is 0 which is treated as SOF_IPC_FRAME_S16_LE. If test uses 24 or 32 bit formats that could lead to wrong consume/produce and invalidate/writeback values.

The problem could be fixed in a separate PR, I just comment it here as your PR adds new place there the fix should be applied.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, thanks for review. I'll take a look at your comments and get back with answers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants